Conversation
…, ILGeneratorImpl.EmitCalli(Type)
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
| public abstract void EmitCalli(OpCode opcode, CallingConvention unmanagedCallConv, Type? returnType, Type[]? parameterTypes); | ||
|
|
||
| /// <summary> | ||
| /// Puts a <see cref="OpCodes.Calli"/> instruction onto the Microsoft intermediate language (MSIL) stream, |
There was a problem hiding this comment.
I kept this description largely the same as the existing overloads. I think it might make more sense to update it though, since as far as I'm concerned the term MSIL was dropped in favor of CIL.
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR implements new function pointer APIs as proposed in issue #75348, enabling better support for function pointer types in IL generation and reflection scenarios.
Changes:
- Adds
Type.MakeFunctionPointerSignatureTypeandType.MakeModifiedSignatureTypestatic factory methods for creating signature types - Implements
ILGenerator.EmitCalli(Type functionPointerType)overload in ILGeneratorImpl for modern function pointer calling conventions - Adds new internal SignatureFunctionPointerType and SignatureModifiedType classes to represent these signature types
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| System.Runtime.cs | Adds public API surface for new function pointer and modified signature type factory methods |
| Type.cs | Implements MakeFunctionPointerSignatureType and MakeModifiedSignatureType factory methods with XML documentation |
| SignatureFunctionPointerType.cs | New internal class representing function pointer signature types with support for calling conventions |
| SignatureModifiedType.cs | New internal class representing types with required/optional custom modifiers |
| SignatureType.cs | Changes UnderlyingSystemType from sealed to virtual to allow SignatureModifiedType to override it |
| ILGenerator.cs | Adds new EmitCalli overload accepting function pointer Type with XML documentation |
| ILGeneratorImpl.cs | Implements the new EmitCalli overload with stack tracking and signature token generation |
| SignatureHelper.cs | Makes WriteSignatureForFunctionPointerType internal and adds logic to unwrap SignatureTypes |
| ModuleBuilderImpl.cs | Adds GetFunctionPointerSignatureToken method to generate standalone signatures for calli instructions |
| DynamicILGenerator.cs | Adds stub TODO implementation for EmitCalli - not yet functional |
| System.Reflection.Emit.ILGeneration.cs | Adds public API surface for new EmitCalli overload |
| Strings.resx | Adds error message for invalid function pointer type arguments |
| System.Private.CoreLib.Shared.projitems | Adds new SignatureFunctionPointerType and SignatureModifiedType to project, plus unrelated whitespace fix |
| SignatureTypes.cs | Adds tests for the new MakeFunctionPointerSignatureType and MakeModifiedSignatureType methods |
| AssemblySaveILGeneratorTests.cs | Adds end-to-end test for EmitCalli with function pointer types |
| Utilities.cs | Adds ClassWithFunctionPointer test helper class |
Comments suppressed due to low confidence (1)
src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs:6
- This using directive appears to be unused. There are no references to System.IO types in this file. Consider removing it to keep the code clean.
using System.Reflection.Metadata;
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureFunctionPointerType.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureModifiedType.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicILGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureFunctionPointerType.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureModifiedType.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs
Show resolved
Hide resolved
…ointerSignatureType
src/libraries/System.Reflection.Emit.ILGeneration/tests/ILGenerator/Emit4Tests.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/SignatureTypes.cs
Show resolved
Hide resolved
|
|
||
| for (int i = 0; i < parameterTypes.Length; i++) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(parameterTypes[i], nameof(parameterTypes)); |
There was a problem hiding this comment.
Do we want to allow void as parameter type?
There was a problem hiding this comment.
(Or uninstantiated generic type definitions.)
There was a problem hiding this comment.
Probably not, I'll add checks for those.
| { | ||
| Debug.Assert(type != null); | ||
|
|
||
| if (type.IsFunctionPointer) |
There was a problem hiding this comment.
I do not see why we are special casing function pointer here. Should it be handled by AddOneArgTypeHelper instead?
If there is a non-obvious reason for this special case, comment would be nice.
There was a problem hiding this comment.
I moved it to AddOneArgTypeHelper.
| if (t.ContainsGenericParameters) | ||
| throw new ArgumentException(SR.Argument_GenericsInvalid, nameof(requiredCustomModifiers)); | ||
|
|
||
| AddElementType((CorElementType)0x22); // ELEMENT_TYPE_CMOD_INTERNAL |
There was a problem hiding this comment.
Is there a reason why we cannot use the ELEMENT_TYPE_CMOD_OPT/ELEMENT_TYPE_CMOD_REQD encoding that's used for arguments in AddDynamicArgument? It looks odd that argument types and return types use different encoding.
There was a problem hiding this comment.
Only reason was that at the start I wanted to avoid adding a DynamicScope parameter. I've cleaned it up now.
| } | ||
| } | ||
| else if (clsArgument.IsFunctionPointer && scope != null) | ||
| { |
There was a problem hiding this comment.
Should we throw NotSupportedException for scope == null? It is better to throw than produce bad binary silently.
I assume that it can be hit with (non-persistent) Reflection.Emit. Is that right? We may want to add a test that validates it is throwing (and not crashing or producing bad output).
There was a problem hiding this comment.
I assume that it can be hit with (non-persistent) Reflection.Emit. Is that right?
Yes, or by just invoking SignatureHelper manually. I added an exception.
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/SignatureHelper.cs
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/SignatureHelper.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/SignatureTypes.cs
Show resolved
Hide resolved
|
|
||
| public override Type MakeFunctionPointerType(Type[]? parameterTypes, bool isUnmanaged = false) | ||
| { | ||
| return Type.MakeFunctionPointerSignatureType(this, parameterTypes, isUnmanaged); |
There was a problem hiding this comment.
Is a signature type sufficient here?
This implements #75348.
MakeFunctionPointerTypeis only implemented for CoreCLR, a Mono implementation will follow.